Support arbitrary role resource version in the editor #51160
Conversation
bl-nero
added
the
no-changelog
bl-nero
force-pushed
the
bl-nero/role-editor-versions
branch
from
e5ba20a to
4d3e7c2
Compare
| options={roleVersionOptions} | ||
| value={value.version} | ||
| onChange={version => onChange({ ...value, version })} | ||
| // rule={precomputed(validation.fields.verbs)} |
| @@ -351,6 +355,7 @@ function KubernetesResourceView({ | |||
| }) { | |||
| const { kind, name, namespace, verbs } = value; | |||
| const theme = useTheme(); | |||
| console.log('ROLE VERSION', value.roleVersion); | |||
| @@ -83,6 +83,7 @@ | |||
| "d3-time-format": "^4.1.0", | |||
| "date-fns": "^2.28.0", | |||
| "history": "^4.9.0", | |||
| "immer": "^10.1.1", | |||
There was a problem hiding this comment.
In the PR description, can you also explain a bit more on why we chose "immer" over other alternatives please?
That information will be valuable for anyone looking for origin of immer as dependency.
| <LabelsInput | ||
| disableBtns={isProcessing} | ||
| labels={value.labels} | ||
| setLabels={labels => onChange?.({ ...value, labels })} |
There was a problem hiding this comment.
onChange is optional? Why?
| options={roleVersionOptions} | ||
| value={value.version} | ||
| onChange={version => onChange({ ...value, version })} | ||
| // rule={precomputed(validation.fields.verbs)} |
There was a problem hiding this comment.
| // rule={precomputed(validation.fields.verbs)} |
dead code
| @@ -159,6 +161,24 @@ test('edits resource access', async () => { | |||
| expect(role.spec.allow.logins).toEqual(['{{internal.logins}}', 'ec2-user']); | |||
| }); | |||
|
|
|||
| test('propagates version info to Kubernetes resource access', async () => { | |||
There was a problem hiding this comment.
maybe rename this test to "triggers v6 validation for kubernetes resource."
I initially got confused otherwise since the test only says propagates version info but we are interested in validation triggered at the end.
| await selectEvent.select(screen.getByLabelText('Kind'), 'Job'); | ||
| await user.click(screen.getByRole('button', { name: 'Create Role' })); | ||
|
|
||
| // Validation should have failed on a Job resource and role v6. |
There was a problem hiding this comment.
Can we assert that some validation error message appears on the screen?
| export type RoleVersionOption = Option<RoleVersion>; | ||
| export const roleVersionOptions: RoleVersionOption[] = ( | ||
| ['v7', 'v6', 'v5', 'v4', 'v3'] as const | ||
| ).map(stringToOption); | ||
| export const roleVersionOptionsMap = optionsToMap(roleVersionOptions); |
There was a problem hiding this comment.
Just generally curious here - why we picked as const and not something like enums?
| verbs: ValidationResult; | ||
| }; | ||
|
|
||
| const validKubernetesKind = ( |
| }; | ||
|
|
||
| export type RoleVersion = 'v3' | 'v4' | 'v5' | 'v6' | 'v7'; |
There was a problem hiding this comment.
Can we define role versions once and reuse them? These are also defined in the standardmodal.ts file
|
Note to self: there's a bug that prevents adding more than one resource. I need to fix it before merging. |
This PR allows changing role resource through the standard editor. It also introduces version-specific validation to the Kubernetes resource access definitions.
Since
updateRoleModelfunction became a bit more involved, I also switched the reducer to use Immer to make it easier to manipulate the domain objects.